Skip to content

make sure setDefaultKeyId resolves even if no change is made - #5485

Merged
andybalaam merged 1 commit into
matrix-org:developfrom
uhoreg:setdefaultkeyid_fix_noop
Aug 17, 2026
Merged

make sure setDefaultKeyId resolves even if no change is made#5485
andybalaam merged 1 commit into
matrix-org:developfrom
uhoreg:setdefaultkeyid_fix_noop

Conversation

@uhoreg

@uhoreg uhoreg commented Aug 14, 2026

Copy link
Copy Markdown
Member

fixes #5474

Make setDefaultKeyId just setAccountData and return, rather than listening for the value to come back down the sync. setAccountData already does that now, when it actually tries to change the value, and does not make an HTTP call if we try to set the account data value to the current value, which means that setDefaultKeyId would never get notified of the value being changed (since it doesn't get changed), and so would never return.

Checklist

  • Tests written for new code (and old code if feasible).
  • New or updated public/exported symbols have accurate TSDoc documentation.
  • Linter and other CI checks pass.
  • Sign-off given on the changes (see CONTRIBUTING.md).

@uhoreg
uhoreg force-pushed the setdefaultkeyid_fix_noop branch from e1f8da3 to 19c5dbb Compare August 14, 2026 21:52
@andybalaam
andybalaam added this pull request to the merge queue Aug 17, 2026
Merged via the queue into matrix-org:develop with commit b4b8d5e Aug 17, 2026
30 checks passed
@richvdh

richvdh commented Aug 18, 2026

Copy link
Copy Markdown
Member

For the archaeological record:

setDefaultKeyId has had a listener on ClientEvent.AccountData to wait for the remote echo ever since 4S was introduced in #832. That was fine at that point, because MatrixClient.setAccountData didn't have any logic to filter out no-op changes, so there would reliably be a remote echo.

Later, #4695 changed MatrixClient.setAccountData so that (a) it did nothing if the change was a no-op; (b) it had its own listener on ClientEvent.AccountData to wait for the remote echo. [Aside: the comments added in that change also assert that there will be no remote echo in the case of a no-op change of account data; it is unclear to me now if that is accurate.] That change made the listener in setDefaultKeyId both redundant and unreliable (since in the case of changes that setAccountData deemed a no-op, there would be no remote echo).

Nevertheless, setDefaultKeyId continued to work ok in the specific case of removing the default key ID (i.e. setting the account data to {}), because, until https://github.com/matrix-org/matrix-js-sdk/pull/5446/changes#diff-7237d115811d917dbc234402ea3e85ced2a568c07005f45344d3a513f30cf68cL295-L301, an account data value of {} was special-cased, so repeated calls to MatrixClient.setAccountData with a value of {} would make an API hit each time.

In other words, between #4695 and #5446, it was working ok, but for all the wrong reasons.

This PR completes the work that should have been done in #4695, by removing the now-redundant listener from setDefaultKeyId.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RustCrypto.resetEncryption() never resolves when secret storage account data is already empty

4 participants